Fix CA1873 false positives for simple params logging args#53027
Fix CA1873 false positives for simple params logging args#53027sebastienros wants to merge 1 commit intomainfrom
Conversation
|
But it is expensive, isn't it? It's allocating an array even when logging is disabled. There's no overload that just takes a string or object or T, only a params array. |
|
Interesting point, then it's really not obvious from the warning itself and the usage. Looking at the code you don't expect it to allocate an array, I know that's what Would it be possible then to have an overload that take a single value for common cases without the array allocation? Otherwise, I will definitely update the code where it's used with guards, knowing that new information. I will interview some colleagues to see if I was alone with that missing piece of knowledge. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a CA1873 analyzer regression introduced in #51839 where simple string reference arguments to logging methods (e.g., logger.LogInformation("Value: {Value}", value)) incorrectly triggered expensive operation warnings.
Changes:
- Modified
IsPotentiallyExpensiveto recursively inspect implicit params array/collection elements instead of treating all params wrappers as expensive - Preserved existing behavior: explicit array/collection creation is still flagged as expensive
- Added regression tests for C# and Visual Basic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/AvoidPotentiallyExpensiveCallWhenLogging.cs | Updated IsPotentiallyExpensive to check elements of implicit params arrays/collections; renamed helper from IsEmptyImplicitParamsArrayCreation to IsImplicitParamsArrayCreation |
| src/Microsoft.CodeAnalysis.NetAnalyzers/tests/Microsoft.CodeAnalysis.NetAnalyzers.UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidPotentiallyExpensiveCallWhenLoggingTests.cs | Added regression tests for C# and VB verifying no diagnostic for simple string reference params |
That's what analyzers are best at, highlighting things you can't see in the code.
Possibly, or a |
I'd be happy to see the analyzer updated with more details about what the cost is, e.g. array allocation, unknown method call, boxing of value types, string allocation (concatenation, interpolation, ...), etc. |
Summary
Fixes a CA1873 regression where simple string-reference logging arguments started producing warnings in 10.0.103.
Fixes #53006.
Problem
After #51839,
IsPotentiallyExpensivetreated compiler-generated implicitparamswrappers (array/collection) as expensive based on wrapper shape alone. This caused false positives for calls like:when
valueis a simple string reference.Fix
AvoidPotentiallyExpensiveCallWhenLoggingAnalyzer.IsPotentiallyExpensive:IArrayCreationOperation(params wrapper) by inspecting elements and only flagging when any contained element is actually expensive.ImplicitReferenceTypeParamsArrayCreation_NoDiagnostic_CSImplicitReferenceTypeParamsArrayCreation_NoDiagnostic_VBTesting
10.0.103: CA1873 appears forlogger.LogInformation("...", value)with stringvalue.10.0.101: same code does not produce CA1873.